-
Notifications
You must be signed in to change notification settings - Fork 13
Automatic subscription to maintained packages #655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automatic subscription to maintained packages #655
Conversation
3df3498 to
08bafbd
Compare
08bafbd to
652cd31
Compare
src/shared/listeners/notify_users.py
Outdated
| notification_reason = [] | ||
|
|
||
| # Check if user is subscribed to any affected packages | ||
| if user in subscribed_users: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that subscribed_users here is a list, which would make this and the ckeck o line 91 a bottleneck for popular packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 7fd96cb
src/shared/listeners/notify_users.py
Outdated
| if maintainers_github: | ||
| maintainer_users = set( | ||
| User.objects.filter( | ||
| username__in=maintainers_github, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to have another JOIN here instead of fetching all GitHub handles into Python just to send them back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 82ee088
src/webview/subscriptions/views.py
Outdated
| if action == "enable": | ||
| if profile.auto_subscribe_to_maintained_packages: | ||
| return self._handle_error( | ||
| request, "Auto-subscription is already enabled." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be an error: if response to one query was lost and user clicked on the button twice, it should still be fine as long as we reach expected state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in dcc4cc3
e3e4aac to
82ee088
Compare
YorikSar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me.
This addresses #647.
This feature introduces a toggle (by default on) to be notified about new suggestions involving packages a user maintains, even if they have not manually added it in the subscriptions center.
Notifications mention that you are notified because you are a maintainer
